Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make inclusive scan safe for cases with leading nulls #7432

Merged
merged 9 commits into from
Feb 26, 2021
Merged

Make inclusive scan safe for cases with leading nulls #7432

merged 9 commits into from
Feb 26, 2021

Conversation

magnatelee
Copy link
Contributor

This PR includes a test that passes a column with leading nulls to scan, which is currently failing, and a fix for the failure.

@magnatelee magnatelee requested a review from a team as a code owner February 23, 2021 22:25
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Feb 23, 2021
@vuule vuule added bug Something isn't working non-breaking Non-breaking change labels Feb 23, 2021
vuule
vuule reviewed Feb 23, 2021
View reviewed changes
cpp/src/reductions/scan.cu Outdated Show resolved Hide resolved
vuule
vuule requested changes Feb 23, 2021
View reviewed changes
cpp/tests/reductions/scan_tests.cpp Outdated Show resolved Hide resolved
cpp/tests/reductions/scan_tests.cpp Outdated Show resolved Hide resolved
@magnatelee
Copy link
Contributor Author

@vuule I fixed set_null_mask. Could you take a look at the code?

vuule
vuule approved these changes Feb 24, 2021
View reviewed changes
cpp/src/bitmask/null_mask.cu
Comment on lines 137 to +138
CUDF_EXPECTS(begin_bit >= 0, "Invalid range.");
CUDF_EXPECTS(begin_bit < end_bit, "Invalid bit range.");
CUDF_EXPECTS(begin_bit <= end_bit, "Invalid bit range.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like these checks should be either merged or we should have more detailed error messages so that the separate checks are useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these assertions have a similar issue:

cudf/cpp/src/bitmask/null_mask.cu

Lines 387 to 388 in f30be67

CUDF_EXPECTS(begin_bit >= 0, "Invalid range.");
CUDF_EXPECTS(begin_bit <= end_bit, "Invalid bit range.");

cudf/cpp/src/bitmask/null_mask.cu

Lines 478 to 479 in f30be67

CUDF_EXPECTS(start >= 0, "Invalid range.");
CUDF_EXPECTS(start <= stop, "Invalid bit range.");

I didn't bother to see if there are more, but I feel improving such checks deserves a separate PR.

@codecov
Copy link

codecov bot commented Feb 24, 2021 •
edited
Loading

Codecov Report

Merging #7432 (12910a3) into branch-0.19 (43b44e1) will increase coverage by 0.46%.
The diff coverage is 96.66%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #7432      +/-   ##
===============================================
+ Coverage        81.80%   82.26%   +0.46%     
===============================================
  Files              101      101              
  Lines            16695    17072     +377     
===============================================
+ Hits             13658    14045     +387     
+ Misses            3037     3027      -10
Impacted Files Coverage Δ
python/cudf/cudf/core/frame.py 89.35% <ø> (+0.09%) ⬆️
python/cudf/cudf/core/column_accessor.py 95.47% <95.65%> (+2.53%) ⬆️
python/cudf/cudf/core/dataframe.py 90.58% <100.00%> (+0.12%) ⬆️
python/cudf/cudf/io/feather.py 100.00% <0.00%> (ø)
python/cudf/cudf/comm/serialize.py 0.00% <0.00%> (ø)
python/cudf/cudf/_fuzz_testing/io.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/struct.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/_version.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_csv.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_orc.py 100.00% <0.00%> (ø)
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2234554...e07a8c0. Read the comment docs.

@kkraus14
Copy link
Collaborator

@gpucibot merge

@kkraus14 kkraus14 added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Feb 26, 2021
@rapids-bot rapids-bot bot merged commit 93c7e0d into rapidsai:branch-0.19 Feb 26, 2021
@magnatelee magnatelee deleted the safe-scan branch February 26, 2021 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vuule vuule vuule approved these changes

@trxcllnt trxcllnt trxcllnt approved these changes

@kkraus14 kkraus14 Awaiting requested review from kkraus14

Assignees
No one assigned
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@magnatelee @kkraus14 @trxcllnt @vuule